Rename candidate center id to registration center #4078

cmadjar · 2018-11-08T14:34:51Z

Brief summary of changes

This PR rename the CenterID field column of the candidate table to RegistrationCenterID, which will correspond to the CenterID at which the candidate was first registered. This should never be changed for a given candidate. This is part 1 of the refactoring of the candidate's CenterID. (See #4058 for details on part 2 of the refactoring).

Associate LORIS-MRI PR

aces/Loris-MRI#341 (@nicolasbrossard will test it)

This resolves issue...

Github? Rename CenterID of the candidate table to RegistrationCenterID (part 1 of candidate's CenterID refactoring) #4057

To test this change...

Make sure the following pages load and work as expected:

dashboard

new profile @ridz1208

candidate list @ridz1208

timepoint list and create timepoint @ridz1208

conflict resolver @ridz1208

genomic_browser @xlecours

imaging browser @davidblader

issue tracker @zaliqarosli

media @zaliqarosli

MRI violations @nicolasbrossard

statistics @ridz1208

survey accounts @sruthymathew123

API v0.0.2 and v.0.0.3-dev @driusan @xlecours

RasinBread dataset and test units @kongtiaowang

tools

Test the SQL patch @PapillonMcGill

Notes for reviewers

I assigned above several people to test various parts of LORIS that were affected by the changes based on my knowledge of who worked on which module. Figured it would be less overwhelming if several people tested one part of the PR instead of having to test everything.

Note that since I don't have any genomics data or media and issue tracker set up, I could not test genomic browser, media or issue tracker.

@driusan @ridz1208 I was not sure who to assign the dashboard, new profile, candidate list, timepoint list, conflict resolvers and the tools. Maybe you can help filling the gap?

Notes for existing projects

RegistrationCenterID should never be changed for a given candidate. The session table will have the information about the latest CenterID the candidate has been seen.
Make sure that c.CenterID is replaced by c.RegistrationCenterID in your project code queries.

zaliqarosli · 2018-11-08T17:51:03Z

@cmadjar i'm trying to apply the patch and get the error Can't write; duplicate key in table. I can't spot anything wrong with the syntax but i'm assuming the FK isn't getting dropped properly?

cmadjar · 2018-11-08T21:21:09Z

@zaliqarosli I modified the patch. I ran with the same issue but could never figure out why it failed when everything is done with one query. I just split the query in 3 statements and it worked for me. Give it a try. Thanks!!

PapillonMcGill · 2018-11-12T12:29:29Z

@cmadjar @zaliqarosli When applying all 3 changes in a single command, the DB server commit only at the semi-column. Thus, when trying to change the column name, the FK is not yet drop, nor the when trying to add it back with the new column name.

Using 3 separate command (with auto-commit), ensure that the FK is first drop before renaming the column and the column is renamed before adding back the FK.

Check the code, look good with of course a few minor thing Travis pointed.

cmadjar · 2018-11-12T15:46:47Z

@PapillonMcGill Thank you for the clarification. That makes a lot of sense!

zaliqarosli · 2018-11-12T20:01:01Z

@cmadjar could you rebase so changes to media module is included? there's a bug with hidden headers on this branch that was fixed on major. other than that, everything else looks good

cmadjar · 2018-11-12T21:34:51Z

@zaliqarosli rebased to the latest version of aces/major. Please review again when you can. Thanks!

modules/dashboard/php/dashboard.class.inc

php/libraries/NDB_BVL_Feedback.class.inc

change request superseeded

modules/candidate_list/php/candidate_list.class.inc

modules/genomic_browser/php/cnv_browser.class.inc

modules/genomic_browser/php/snp_browser.class.inc

php/libraries/NDB_BVL_Feedback.class.inc

cmadjar

@zaliqarosli Thank you for your review!! All your comments should be taken care off. The only ones I did not change anything are for the genomic browser. I don't think genomic data is linked to a session but only to a candidate. @xlecours Is that right?

modules/candidate_list/php/candidate_list.class.inc

modules/genomic_browser/php/cnv_browser.class.inc

modules/genomic_browser/php/snp_browser.class.inc

php/libraries/NDB_BVL_Feedback.class.inc

cmadjar · 2018-11-19T20:16:04Z

@zaliqarosli Sorry, I dismissed your review by merging @xlecours PR to my branch (I missed one instance of CenterID in the genomic module)

xlecours · 2018-11-19T20:24:54Z

@cmadjar I concur. There is no session liked to genomic data; yet

kongtiaowang

c.centerID should be c.RegistrationCenterID

kongtiaowang · 2019-01-24T19:34:18Z

modules/mri_violations/php/mri_violations.class.inc

@@ -246,7 +246,7 @@ class Mri_Violations extends \NDB_Menu_Filter_Form
                AND mpvs.CandID = s.CandID
            )
            LEFT JOIN psc p
-            ON (p.CenterID = c.CenterID)
+            ON (p.CenterID = s.CenterID)


It should be " ON (p.CenterID = c. RegistrationCenterID)"

cmadjar

@kongtiaowang actually I modified the join to join to the session CenterID in the queries you mentioned because I think it is best to use the CenterID of the session table here (s.CenterID) since a scan is linked to a session and the session's CenterID will be more informative about where the scans was performed than the CenterID from the candidate table if a candidate belongs to multiple sites.

I am not sure either this is why travis is failing. The Travis error is:

UnknownServerException: document.querySelector(...) is null
Build info: version: '2.53.1', revision: 'a36b8b1', time: '2016-06-30 17:37:03'
System info: host: '4e44a21cd803', ip: '172.18.0.2', os.name: 'Linux', os.arch: 'amd64', os.version: '4.4.0-101-generic', java.version: '1.8.0_91'
Driver info: driver.version: unknown
/app/vendor/facebook/webdriver/lib/WebDriverExceptions.php:79
/app/vendor/facebook/webdriver/lib/remote/HttpCommandExecutor.php:268
/app/vendor/facebook/webdriver/lib/remote/RemoteWebDriver.php:491
/app/vendor/facebook/webdriver/lib/remote/RemoteWebDriver.php:270
/app/modules/mri_violations/test/mri_violationsTest.php:538
/app/modules/mri_violations/test/mri_violationsTest.php:448

does not seem to be related with the renaming of the CenterID field. Does it? Not sure either why this means?

ridz1208 · 2019-01-25T13:50:46Z

@kongtiaowang I agree with @cmadjar s.centerid makes more sens here

ridz1208 · 2019-01-25T13:52:41Z

@cmadjar if you manually test MRI violations is everything working fine ?

thats where travis seems to be having issues

cmadjar · 2019-01-25T15:06:33Z

@ridz1208 Indeed, the MRI violation module seems to work OK aside from what @nicolasbrossard reported which is a bug on major unrelated to my changes:

Fyi, from @nicolasbrossard : The MRI violations module loads and looks OK, except for a display bug in the MRI violations results table that existed prior to Cecile's PR. Approving.

ridz1208 · 2019-01-25T18:05:23Z

@cmadjar
Parse error: syntax error, unexpected ';', expecting ')' in /app/modules/mri_violations/test/mri_violationsTest.php on line 90

EDIT: I fixed it directly on github

cmadjar · 2019-01-25T18:15:00Z

@PapillonMcGill @zaliqarosli @nicolasbrossard @ridz1208 @xlecours A force push deleted your approval review. Could you look at that PR one last time? Thanks!

zaliqarosli · 2019-01-25T19:54:56Z

@cmadjar i'm getting a lot of issues with the issue tracker :( so i'm going to properly test again

driusan · 2019-01-28T18:27:42Z

@cmadjar is this good to go already??

cmadjar · 2019-01-28T19:04:10Z

@driusan yes pretty please :). This way I can start working on phase 2 this week!

…rojectID to c.RegistrationProjectID (#4919) This does 3 related changes to add user projects to LORIS: 1. Add the ProjectID field to sessions in the same manner as centers are affiliated to sessions for future development of project permissions 2. Change the candidate.ProjectID to candidate.RegistrationProjectID to remain consistent with the centerID changes done in #4078 3. Add necessary foreign keys to the SQL and fix existing fields to support the foreign keys from all tables.

cmadjar changed the base branch from minor to major November 8, 2018 14:35

cmadjar mentioned this pull request Nov 12, 2018

Rename candidate CenterID to RegistrationCenterID aces/Loris-MRI#341

Merged

3 tasks

cmadjar force-pushed the rename_candidate_CenterID_to_RegistrationCenterID branch from 40f41f9 to 54f739c Compare November 12, 2018 21:30

PapillonMcGill previously requested changes Nov 13, 2018

View reviewed changes

modules/dashboard/php/dashboard.class.inc Show resolved Hide resolved

php/libraries/NDB_BVL_Feedback.class.inc Outdated Show resolved Hide resolved

php/libraries/NDB_BVL_Feedback.class.inc Outdated Show resolved Hide resolved

zaliqarosli requested changes Nov 19, 2018

View reviewed changes

cmadjar commented Nov 19, 2018

View reviewed changes

zaliqarosli previously approved these changes Nov 19, 2018

View reviewed changes

cmadjar dismissed zaliqarosli’s stale review via db48ab1 November 19, 2018 20:14

zaliqarosli previously approved these changes Nov 19, 2018

View reviewed changes

PapillonMcGill previously approved these changes Nov 20, 2018

View reviewed changes

zaliqarosli added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Nov 26, 2018

ridz1208 assigned xlecours and ridz1208 Jan 11, 2019

nicolasbrossard previously approved these changes Jan 22, 2019

View reviewed changes

kongtiaowang requested changes Jan 24, 2019

View reviewed changes

cmadjar commented Jan 24, 2019

View reviewed changes

cmadjar added the "Help! I don't understand Travis!" PR is having a beef with TRAVIS. Someone needs to help label Jan 24, 2019

kongtiaowang dismissed stale reviews from nicolasbrossard and xlecours via 28f3d96 January 25, 2019 15:55

kongtiaowang force-pushed the rename_candidate_CenterID_to_RegistrationCenterID branch from 0013677 to 28f3d96 Compare January 25, 2019 15:55

cmadjar force-pushed the rename_candidate_CenterID_to_RegistrationCenterID branch from 28f3d96 to 0013677 Compare January 25, 2019 17:34

fixing the MRI violation test script

1335346

Update mri_violationsTest.php

7f310c6

ridz1208 approved these changes Jan 25, 2019

View reviewed changes

kongtiaowang approved these changes Jan 25, 2019

View reviewed changes

kongtiaowang removed the "Help! I don't understand Travis!" PR is having a beef with TRAVIS. Someone needs to help label Jan 25, 2019

zaliqarosli approved these changes Jan 25, 2019

View reviewed changes

xlecours approved these changes Jan 25, 2019

View reviewed changes

PapillonMcGill approved these changes Jan 28, 2019

View reviewed changes

driusan merged commit cfb46c7 into aces:major Jan 28, 2019

cmadjar deleted the rename_candidate_CenterID_to_RegistrationCenterID branch January 30, 2019 20:33

ridz1208 added this to the 21.0.0 milestone Feb 11, 2019

ridz1208 mentioned this pull request Jul 4, 2019

[Project] Associate projects to sessions + fix/add foreign keys + c.ProjectID to c.RegistrationProjectID #4919

Merged

34 tasks

christinerogers mentioned this pull request Aug 1, 2019

Post-release 21 Documentation updates #4995

Closed

27 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename candidate center id to registration center #4078

Rename candidate center id to registration center #4078

cmadjar commented Nov 8, 2018 •

edited by zaliqarosli

Loading

zaliqarosli commented Nov 8, 2018

cmadjar commented Nov 8, 2018

PapillonMcGill commented Nov 12, 2018

cmadjar commented Nov 12, 2018

zaliqarosli commented Nov 12, 2018 •

edited

Loading

cmadjar commented Nov 12, 2018

cmadjar left a comment

cmadjar commented Nov 19, 2018

xlecours commented Nov 19, 2018 •

edited

Loading

kongtiaowang left a comment

kongtiaowang Jan 24, 2019

cmadjar left a comment

ridz1208 commented Jan 25, 2019

ridz1208 commented Jan 25, 2019

cmadjar commented Jan 25, 2019

ridz1208 commented Jan 25, 2019 •

edited

Loading

cmadjar commented Jan 25, 2019

zaliqarosli commented Jan 25, 2019

driusan commented Jan 28, 2019

cmadjar commented Jan 28, 2019

Rename candidate center id to registration center #4078

Rename candidate center id to registration center #4078

Conversation

cmadjar commented Nov 8, 2018 • edited by zaliqarosli Loading

Brief summary of changes

Associate LORIS-MRI PR

This resolves issue...

To test this change...

Notes for existing projects

zaliqarosli commented Nov 8, 2018

cmadjar commented Nov 8, 2018

PapillonMcGill commented Nov 12, 2018

cmadjar commented Nov 12, 2018

zaliqarosli commented Nov 12, 2018 • edited Loading

cmadjar commented Nov 12, 2018

cmadjar left a comment

Choose a reason for hiding this comment

cmadjar commented Nov 19, 2018

xlecours commented Nov 19, 2018 • edited Loading

kongtiaowang left a comment

Choose a reason for hiding this comment

kongtiaowang Jan 24, 2019

Choose a reason for hiding this comment

cmadjar left a comment

Choose a reason for hiding this comment

ridz1208 commented Jan 25, 2019

ridz1208 commented Jan 25, 2019

cmadjar commented Jan 25, 2019

ridz1208 commented Jan 25, 2019 • edited Loading

cmadjar commented Jan 25, 2019

zaliqarosli commented Jan 25, 2019

driusan commented Jan 28, 2019

cmadjar commented Jan 28, 2019

cmadjar commented Nov 8, 2018 •

edited by zaliqarosli

Loading

zaliqarosli commented Nov 12, 2018 •

edited

Loading

xlecours commented Nov 19, 2018 •

edited

Loading

ridz1208 commented Jan 25, 2019 •

edited

Loading